Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some improvements for packaged build [wip] #82

Closed
wants to merge 3 commits into from

Conversation

noiseless
Copy link

@noiseless noiseless commented Dec 28, 2021

I would like to share you my suggestions for improving TG_OWT_PACKAGED_BUILD.

Here they are:

  1. cherry-picked include cmake rules for third party libraries only if it needed #66 && fixed merge conflict
  2. This patch is an adaptation of Vitaly Lipatov's work lav@altlinux.ru for the latest versions of tg_owt. These changes are a credit to him and are now, as far as I can see, already being used to build alt linux packages.

The patch adds cmake rules for building tg_owt with libyuv from the system/packages.

  1. In this patch I fix the paths to the libyuv header files. Similar patch for tgcalls (separately they make little sense - packaged build will break).

Testing.

To test my changes I did the following:

  1. I have done the build according to the "Build instructions for Linux using Docker".

To apply my changes, I replaced these lines in centos_env/Dockerfile from

RUN git remote add origin $GIT/desktop-app/tg_owt.git
RUN git fetch --depth=1 origin b02478677baac6d563589f216800ff9cea0fd65c

to

RUN git remote add origin $GIT/noiseless/tg_owt.git
RUN git fetch --depth=1 origin c5fc0b3ac814b599c161fcc93e62e9a267c86662

and also edited .gitmodules:

[submodule "Telegram/ThirdParty/tgcalls"]
    path = Telegram/ThirdParty/tgcalls
    url = https://github.com/noiseless/tgcalls.git
    branch = system-libyuv

The build was successful.
2) I did a packaged build under OpenBSD. Tested situations where libyuv is installed on the system and where it is not there, but everything you need is in ./src/third_party/libyuv.

In the first case, tg_owt was built with libyuv from the package, in the second, from ./src/third_party/libyuv, i.e. everything is as expected.

What do you think of my patches? If necessary, I am ready to improve them according to your wishes.
Perhaps I should test some other build variants?

Thanks anyway.

@ilya-fedin
Copy link
Contributor

ilya-fedin commented Dec 29, 2021

  • In the last patch I turn off cmake/{libabsl,libopenh264,libusrsctp,libvpx,libyuv}.cmake for packaged builds.

Oh. Every time someone tries to do this, snap package breaks. So this would need extensive testing across all the building configuration.

include(cmake/libusrsctp.cmake)
include(cmake/libvpx.cmake)
include(cmake/libyuv.cmake)
if (NOT TG_OWT_PACKAGED_BUILD)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks the fallback logic when a library is not found

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that can't be right.
#85 should do the trick, though.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is wrong, as Ilya has already told me. 5986975 should fix that.
(However, there are linking problems with snap pack builds there; I haven't had a chance to reproduce it yet, but I'm still going to)

Integrated the Vitaly Lipatov's (lav@altlinux.ru) patch.
It allows to build tg_owt with the system libyuv. If libyuv is
not installed on the system, the build will be performed with
libyuv from src/third_party/.
Also fixed build in non-packaged mode.
@noiseless noiseless changed the title Some improvements for packaged build Some improvements for packaged build [wip] Dec 30, 2021
@ilya-fedin
Copy link
Contributor

Is there still any work? Maybe close the PR and re-open when you would have time?

@noiseless
Copy link
Author

reasonably

@noiseless noiseless closed this Feb 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants